Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added initial WebXR entry UI and export target. #312

Merged
merged 1 commit into from
Jan 7, 2023

Conversation

Malcolmnixon
Copy link
Collaborator

@Malcolmnixon Malcolmnixon commented Jan 4, 2023

This pull request adds WebXR support to the godot-xr-tools demo project.

Note - this turns on Thread support in the WebXR HTML5 export presets as the godot-xr-tools project is set up to use resource_queue to load resources in a worker thread.

@Malcolmnixon
Copy link
Collaborator Author

@dsnopek Could you review this PR. I played with it on my Quest 2 and it seems to work fine.

Copy link
Member

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but I think @dsnopek should review too :)

@BastiaanOlij BastiaanOlij added the enhancement New feature or request label Jan 4, 2023
@BastiaanOlij BastiaanOlij added this to the 3.2.0 milestone Jan 4, 2023
@dsnopek
Copy link
Collaborator

dsnopek commented Jan 4, 2023

I attempted to test this really quick but was getting an error about first_person_controller_vr.tscn being missing? But I don't think that has anything to do with this PR.

Just from skimming the code, this looks great!

@Malcolmnixon
Copy link
Collaborator Author

I attempted to test this really quick but was getting an error about first_person_controller_vr.tscn being missing? But I don't think that has anything to do with this PR.

Just from skimming the code, this looks great!

This is because we have a dependency on the godot_openxr library for that scene. @BastiaanOlij can you think of any way of removing this dependency when it's a WebXR only situation?

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 4, 2023

Or maybe xr-tools should have it's own copy of the first person controller scene? This would come up with any other XR API as well.

@BastiaanOlij
Copy link
Member

BastiaanOlij commented Jan 4, 2023

Owh yeah, using the first_controller_person.tscn from the plugin is a convenience and also best practice for anyone targeting OpenXR, but it is not a requirement.

For a multi platform setup one could even argue that you don't want to go this way. There is nothing stopping us from just adding a normal ARVROrigin/ARVRCamera/ARVRController setup to the main scene and reproducing the initialisation script in the demo itself. It can then even be a script where a lot of overlapping things are used. You would end up with an init function like (Godot 3 pseudocode, I think this would be worth implementing in both 3 and 4):

var xr_interface : ARVRInterface

func _setup_for_openxr():
    ## do specific stuff for openxr
    ...

func _setup_for_webxr():
    ## do specific stuff for webxr
    ...

func _generic_setup():
    var vp = get_viewport()  
    vp.arvr = true

    ## Should obtain headset FPS in our _setup_for_xyz() functions and use that here but with a multiplier
    ## physics should really run at something like double the frame rate of the headset so lower margins become stable
    Engine.iterations_per_second = 90

func initialise() -> bool:
    xr_interface = ARVRServer.find_interface('OpenXR')
    if xr_interface and xr_interface.initialize():
        _setup_for_openxr()
        _generic_setup()
        return true

    xr_interface = ARVRServer.find_interface('WebXR')
    if xr_interface and xr_interface.initialize():
        _setup_for_webxr()
        _generic_setup()
        return true

    xr_interface = null
    return false

TBH at this point in time, I don't even mind that being part of the plugin

@Malcolmnixon
Copy link
Collaborator Author

There are a few things the first_person_controller_vr.tscn from godot_openxr includes:

  • The first_person_controller_vr.gd which you were mentioning above and includes passthrough enabling
  • "configuration" which uses OpenXRConfig.gdns and provides:
    • View Configuration
    • Form Factor
    • Play Space Type
    • Action Sets
    • Interaction Profiles
  • Controllers using controller.gd which hides the controllers when the tracking confidence drops to none

I'm guessing the "configuration" node might be necessary, but it also can't be present in the scene if it's a WebXR only version and godot_openxr hasn't been placed in the assets folder.

I assume what triggers Godot to actually use godot_openxr is the presence of the "godot_openxr.gdnlib" with the "singleton" option enabled.

@Malcolmnixon Malcolmnixon force-pushed the webxr-support branch 2 times, most recently from ce0eb84 to 2753d3e Compare January 5, 2023 04:06
@Malcolmnixon
Copy link
Collaborator Author

I had to rework this pull request fairly heavily to support WebXR without having the godot_openxr asset present. The change includes a new StartXR node which can be added to the staging scene:
image

Note that the scene now has a standard ARVROrigin rather than using the godot_openxr FPController scene.

This node will detect both OpenXR and WebXR and start the appropriate interface. It has a few basic properties which we could extend later:
image

@Malcolmnixon
Copy link
Collaborator Author

@dsnopek Could you try this again.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 5, 2023

Works perfectly in my testing!

Skimming the diff, the rework seems great to me too :-)

@dsnopek dsnopek self-requested a review January 5, 2023 23:14
@BastiaanOlij
Copy link
Member

I'm really liking this, I am wondering if we shouldn't go a little further and just add the logic into our staging scene in the plugin itself. The reason I didn't add the ARVROrigin node in there was because it would be possible to use it with different plugins, but seeing OpenXR and WebXR are really the only ones we'll likely use and we now have most of the logic in the plugin already anyway, might as well just put the whole thing in staging and make it really easy to use it.

@Malcolmnixon
Copy link
Collaborator Author

Are there any use-cases where projects would want to start XR but not use the staging infrastructure?

We could move the StartXR and the ARVROrigin/camera/controllers into the plugin staging; so staging-based projects get it for free, but non-staging projects can drop in the StartXR node.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 6, 2023

Are there any use-cases where projects would want to start XR but not use the staging infrastructure?

Personally, I don't always use the staging infrastructure when prototyping something simple, or just testing an individual scene.

We could move the StartXR and the ARVROrigin/camera/controllers into the plugin staging; so staging-based projects get it for free, but non-staging projects can drop in the StartXR node.

This sounds good!

@Malcolmnixon
Copy link
Collaborator Author

I've updated the PR with moving the StartXR node as well as the ARVROrigin/camera/controllers into the base staging scene. This makes creating a customized staging scene significantly easier - in fact the base one will work unless the developer wishes to customize the prompt_for_continue and pause/resume logic.

The StartXR node is still available for projects which don't want to use staging.

@Malcolmnixon Malcolmnixon force-pushed the webxr-support branch 2 times, most recently from cb8f6be to 9f0cc85 Compare January 6, 2023 18:26
@Malcolmnixon
Copy link
Collaborator Author

Merged with Godot 4 equivalent functionality.

Copy link
Member

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nitpicking things but I think this is a great change making it easier to start with a cross platform solution.

progress = 0.0

[node name="Scene" type="Spatial" parent="."]

[node name="Tween" type="Tween" parent="."]

[node name="ARVROrigin" type="ARVROrigin" parent="."]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should rename the nodes to XROrigin and XRCamera, might make things more recognizable between 3 and 4? Or am I over thinking it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has benefits for compatibility, but it may also be a source of confusion as the developers will still have to use the ARVRxxxx names in code. If we choose to do this then I would probably make it a separate PR as we'd want to sweep through other files and comments.

addons/godot-xr-tools/xr/start_xr.gd Outdated Show resolved Hide resolved
addons/godot-xr-tools/xr/start_xr.gd Outdated Show resolved Hide resolved
addons/godot-xr-tools/xr/start_xr.gd Outdated Show resolved Hide resolved
Added thread support to handle resource queue loading.

Added new StartXR scene to manage starting the OpenXR or WebXR instance. Fixed loading_screen so it doesn't produce NANs when the headset is off for a while.

Fixed minor gdlint warnings.

Move ARVROrigin/camera/controllers into base staging scene. Move StartXR into base staging scene. Modified base staging scene to emit xr_started/xr_ended signals.

Synchronized with Godot 4 equivalent functionality and events.

Formatting fixes

Synchronize more changes with Godot 4 version.

Applied changes from code review feedback.
@Malcolmnixon Malcolmnixon merged commit 552fe72 into GodotVR:master Jan 7, 2023
@Malcolmnixon Malcolmnixon deleted the webxr-support branch January 7, 2023 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants